Skip to content

aamtests (EdgeDriver): Compute correct pid & Enable running IA2 tests - #60733

Merged
spectranaut merged 6 commits into
web-platform-tests:masterfrom
yezhizhen:patch-1
Jun 30, 2026
Merged

aamtests (EdgeDriver): Compute correct pid & Enable running IA2 tests#60733
spectranaut merged 6 commits into
web-platform-tests:masterfrom
yezhizhen:patch-1

Conversation

@yezhizhen

@yezhizhen yezhizhen commented Jun 18, 2026

Copy link
Copy Markdown
Contributor
  • pid was always missing for EdgeDriver. Now we compute it, with same capability as ChromeDriver goog:processID
  • The test was failing as we were searching window handle by name "MicrosoftEdge". However, it has some weird name data:,-personal-microsoft​edge instead. We use a more robust approach, searching by pid just identified.

Testing:

def test_ia2(ia2, session, inline):
passes on Windows, same as Chrome.

cc @spectranaut @xiaochengh

@spectranaut spectranaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for looking into and fixing this :)

return False

user32.EnumWindows(check_window_name, LPARAM(0))
user32.EnumWindows(check_pid, LPARAM(0))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of deleting/replacing check_window_name, can we instead:

Suggested change
user32.EnumWindows(check_pid, LPARAM(0))
if pid:
user32.EnumWindows(check_pid, LPARAM(0))
else:
user32.EnumWindows(check_window_name, LPARAM(0))

If someone wants to test a different browser on windows (see all the browsers that WPT supports: https://github.com/web-platform-tests/wpt/tree/master/tools/wptrunner/wptrunner/browsers) -- they might need to find the browser by name, if their webdriver implementation does not return PID. So it would be nice to leave this fall back :)

@yezhizhen yezhizhen Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Done in 22c48f0

@cookiecrook
cookiecrook requested a review from benbeaudry June 25, 2026 17:20

@benbeaudry benbeaudry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

One thought: Microsoft Edge does indeed add more context in its window name so if we do a full comparison, it is bound to fail. But we always append "Microsoft Edge" at the end, so a substring search could work too, but could become a false positive if another browser's name includes the tab name and the browser name and the open tab's name contains "Microsoft Edge". All of this to say that relying on the name alone isn't robust, and I much prefer the new approach based on PID. Thank you!

@HaTheo

HaTheo commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

@yezhizhen Thanks for putting this together! I actually opened a very similar PR a few days ago (#60811).

Looking at your implementation, I noticed a few differences in how we approached this. In hindsight, my approach for a couple of these was a bit over-engineered, but the first point might still be worth considering:

  1. I returned "edge" as the product_name instead of "MicrosoftEdge". "edge" seems to be what was used across most of the codebase, so using it here might help prevent issues down the line. And since it's mostly checked if that product_name in window name etc.
  2. I handled both"MicrosoftEdge" and "edge". After testing, I realized these extra checks weren't actually necessary as it consistently gets MicrosoftEdge from the webdriver, so your approach here is totally fine.
  3. I initially checked for an ms:processId key, expecting it given the other ones before debugging and realizing it only had a goog:processId. Anyway, making my extra checks isn't needed, I left it in incase it case as it seemed strange and figured it might change overtime... but your approach is simpler, and less over-engineered.

Overall, your changes look great to me! Aside from the supernitty #1, of potentially updating the returned product name to "edge" for consistency. I will actually abandon my change to keep things simple, so it doesn't conflict with this in any way, since they functionally do the same thing.

@yezhizhen
yezhizhen requested a review from spectranaut June 30, 2026 03:14

@spectranaut spectranaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much!! :)

@spectranaut

spectranaut commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@Ms2ger, can you merge this one? The Safari failure is unrelated.

edit: nevermind, I could merge this one for whatever reason!

@spectranaut
spectranaut merged commit 4767daa into web-platform-tests:master Jun 30, 2026
27 of 28 checks passed
@yezhizhen
yezhizhen deleted the patch-1 branch July 1, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants